Skip to content

fix(roadmap): harden generation startup and artifact parsing#1958

Open
VDT-91 wants to merge 3 commits intoAndyMik90:developfrom
VDT-91:fix/roadmap-generation-reliability
Open

fix(roadmap): harden generation startup and artifact parsing#1958
VDT-91 wants to merge 3 commits intoAndyMik90:developfrom
VDT-91:fix/roadmap-generation-reliability

Conversation

@VDT-91
Copy link
Collaborator

@VDT-91 VDT-91 commented Mar 16, 2026

Summary

  • propagate abort signals through roadmap client creation, auth resolution, OAuth refresh, and provider fetches so roadmap startup respects timeouts and user stop actions
  • map internal discovery / features queue phases to the UI-facing discovering / generating phases and add focused coverage for that bridge
  • repair invalid control characters in roadmap JSON artifacts before parsing, persist repaired files back to disk, and preserve valid Unicode text such as æ, ø, and å

Why

Roadmap generation had two concrete reliability failures:

  1. startup could outlive the user-visible timeout while nested auth refresh work kept running
  2. discovery could fail three times on otherwise-usable JSON artifacts because the model emitted illegal control characters inside multilingual text

Testing

  • npm run test -- src/main/utils/__tests__/json-repair.test.ts src/main/ai/runners/roadmap.test.ts src/main/__tests__/agent-queue.test.ts src/main/claude-profile/token-refresh.test.ts
  • npm run typecheck

Summary by CodeRabbit

Release Notes

  • New Features

    • Added cancellation support for authentication and long-running operations, allowing users to interrupt operations in progress.
    • Implemented timeout protection for client creation and token refresh operations.
  • Bug Fixes

    • Improved JSON file handling with automatic repair and normalization of invalid characters.
  • Tests

    • Added comprehensive test coverage for roadmap generation, phase mapping, and token refresh workflows with abort scenarios.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 16, 2026

📝 Walkthrough

Walkthrough

Introduces abort/cancellation support throughout authentication and client creation flows, adds per-request and total timeout mechanisms for OAuth token refresh, implements JSON repair with illegal control character stripping, and adds phase mapping for roadmap generation events.

Changes

Cohort / File(s) Summary
Abort Signal Infrastructure
apps/desktop/src/main/ai/auth/resolver.ts, apps/desktop/src/main/ai/auth/types.ts, apps/desktop/src/main/claude-profile/token-refresh.ts
Threads abort signal support through authentication resolution with throwIfAborted checks, early-abort guards, and propagation into credential resolution. Adds per-request timeout and total maximum duration constants for OAuth token refresh with retry backoff handling.
Client Creation & Factory
apps/desktop/src/main/ai/client/factory.ts, apps/desktop/src/main/ai/client/types.ts, apps/desktop/src/main/ai/runners/roadmap.ts
Propagates abortSignal through client creation paths, adds CLIENT_CREATION_TIMEOUT_MS with AbortController-based timeout handling, and introduces readAndRepairJsonFile helper for robust JSON loading with repair and atomic writes.
OAuth Token Management
apps/desktop/src/main/ai/providers/oauth-fetch.ts, apps/desktop/src/main/claude-profile/token-refresh.test.ts
Extends OAuth token refresh functions (refreshOAuthToken, ensureValidOAuthToken) to accept and propagate abortSignal, adds abort error handling, and verifies abort propagation without retries in tests.
Roadmap Phase Mapping
apps/desktop/src/main/agent/agent-queue.ts, apps/desktop/src/main/agent/__tests__/agent-queue.test.ts
Introduces phaseMap to translate runner phases (discovery→discovering, features→generating) for progress event emission and includes unit tests validating phase order and message content.
JSON Repair & Handling
apps/desktop/src/main/utils/json-repair.ts, apps/desktop/src/main/utils/__tests__/json-repair.test.ts
Adds control character stripping step to repairJson to remove illegal JSON control characters (U+0000–U0008, U+000B, U+000C, U+000E–U+001F) while preserving valid Unicode, tabs, newlines, and carriage returns.
Roadmap Runner Tests
apps/desktop/src/main/ai/runners/roadmap.test.ts
Introduces tests for runRoadmapGeneration covering client creation timeout (120 seconds) and user-initiated cancellation scenarios with AbortSignal abort verification.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Suggested labels

bug, size/M

Poem

🐰 Hops through signals and timeouts with care,
Aborting when needed with abort-aware flair,
JSON's now cleaner, phases properly named,
The roadmap shines bright—cancellation untamed!
Our commits repair what the data might break,
A hop, skip, and abort for performance's sake. 🌿

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main objective: hardening roadmap generation startup (abort signal propagation for timeouts) and artifact parsing (JSON repair for control characters).

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📝 Coding Plan
  • Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the reliability and responsiveness of the roadmap generation process. It addresses critical issues where roadmap startup could exceed user-defined timeouts due to uncancelable background authentication tasks, and where discovery failed because of malformed JSON artifacts containing illegal control characters. By integrating abort signals throughout the authentication and client creation pipeline, and by introducing a resilient JSON repair mechanism, the changes ensure that roadmap generation respects user cancellations and successfully processes multilingual data.

Highlights

  • Robust Cancellation Handling: Propagated abort signals through roadmap client creation, authentication resolution, OAuth refresh, and provider fetches to ensure timely termination and user control, preventing operations from outliving their intended timeouts.
  • Improved Progress Reporting: Mapped internal roadmap generation phase names (e.g., 'discovery', 'features') to user-facing UI phases ('discovering', 'generating') for more consistent and understandable progress reporting.
  • Enhanced JSON Artifact Repair: Implemented robust JSON artifact parsing by repairing invalid control characters and preserving valid Unicode text (such as 'æ', 'ø', and 'å'), then persisting the repaired files back to disk to prevent discovery failures.
Changelog
  • apps/desktop/src/main/tests/agent-queue.test.ts
    • Added a test to verify the mapping of internal roadmap runner phases to frontend UI phases.
  • apps/desktop/src/main/agent/agent-queue.ts
    • Updated the roadmap progress mapping logic to translate internal 'discovery' and 'features' phases to 'discovering' and 'generating' for UI display.
  • apps/desktop/src/main/ai/auth/resolver.ts
    • Introduced a 'throwIfAborted' utility function.
    • Integrated 'AbortSignal' propagation into 'resolveFromProviderAccount', 'ensureValidOAuthToken', 'ensureValidToken', 'resolveAuth', 'resolveAuthFromQueue', and 'resolveCredentialsForAccount' functions.
  • apps/desktop/src/main/ai/auth/types.ts
    • Added an optional 'abortSignal' property to the 'AuthResolverContext' interface.
  • apps/desktop/src/main/ai/client/factory.ts
    • Modified 'createAgentClient' and 'createSimpleClient' to accept and propagate an 'abortSignal' during client creation and authentication resolution.
  • apps/desktop/src/main/ai/client/types.ts
    • Added an optional 'abortSignal' property to the 'SimpleClientConfig' interface.
  • apps/desktop/src/main/ai/providers/oauth-fetch.ts
    • Updated 'refreshOAuthToken' and 'ensureValidOAuthToken' to accept and utilize an 'abortSignal' for cancellation during network requests and token refresh attempts.
  • apps/desktop/src/main/ai/runners/roadmap.test.ts
    • Added new tests to confirm that roadmap client creation correctly aborts when a timeout elapses or a user cancellation signal is received.
  • apps/desktop/src/main/ai/runners/roadmap.ts
    • Implemented a client creation timeout to prevent indefinite hangs during authentication.
    • Introduced 'throwIfAborted' for early exit based on abort signals.
    • Replaced 'safeParseJson' with a new 'readAndRepairJsonFile' function that automatically repairs and persists malformed JSON artifacts.
  • apps/desktop/src/main/claude-profile/token-refresh.test.ts
    • Added a test case to verify that 'refreshOAuthToken' propagates caller abort signals without retrying.
  • apps/desktop/src/main/claude-profile/token-refresh.ts
    • Introduced 'TOKEN_REFRESH_TIMEOUT_MS' and 'MAX_OAUTH_REFRESH_TOTAL_DURATION_MS' constants for better timeout management.
    • Added 'throwIfAborted', 'delay', and 'createRefreshRequestSignal' utilities.
    • Integrated 'abortSignal' into the OAuth token refresh process to allow for cancellation during network requests and delays.
  • apps/desktop/src/main/utils/tests/json-repair.test.ts
    • Added test cases to ensure the 'repairJson' function correctly strips illegal control characters while preserving valid Unicode and handles real-world JSON parsing issues.
  • apps/desktop/src/main/utils/json-repair.ts
    • Enhanced the 'repairJson' function to strip illegal JSON control characters (U+0000 to U+001F, excluding tabs, line feeds, and carriage returns) while preserving valid Unicode.
Activity
  • Added comprehensive unit tests for 'json-repair' utilities, 'roadmap' runner client creation, 'agent-queue' roadmap progress mapping, and 'claude-profile' token refresh abort propagation.
  • Ensured type-checking passes across all modified files.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces significant hardening for the roadmap generation process by propagating abort signals for timeouts and user cancellations, and by adding a robust JSON repair mechanism for artifacts. The changes are well-implemented and include relevant tests. My review includes a few suggestions to further improve error handling and maintainability by enhancing diagnostics, ensuring consistent error propagation, and reducing code duplication.

Comment on lines +121 to +123
} catch {
return null;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This broad catch block silently swallows all errors that might occur within the try block, including file system errors (e.g., from readFileSync, writeFileSync, renameSync) and JSON parsing/repair errors. While returning null for invalid JSON is intended, masking potentially critical file system errors (like permission issues) can make debugging very difficult. I recommend logging the error before returning null to improve diagnostics.

  } catch (error) {
    console.error(`[readAndRepairJsonFile] Failed to read, repair, or parse ${filePath}:`, error);
    return null;
  }

Comment on lines +41 to +50
function throwIfAborted(signal?: AbortSignal): void {
if (!signal?.aborted) return;

const reason = signal.reason;
if (reason instanceof Error) {
throw reason;
}

throw new Error(typeof reason === 'string' ? reason : 'Aborted');
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This throwIfAborted helper function is also defined in apps/desktop/src/main/ai/auth/resolver.ts and apps/desktop/src/main/claude-profile/token-refresh.ts. To improve maintainability and avoid code duplication, consider extracting it into a shared utility file, for example in apps/desktop/src/main/utils/.

Comment on lines +525 to +531
throw new Error('Aborted');
}
if (clientCreationTimeoutController.signal.aborted) {
throw new Error('Client creation timed out — check your authentication credentials');
}
throw error;
} finally {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The error handling for aborts could be improved. When the external abortSignal is aborted, this catch block throws a new generic Error('Aborted'), which loses the original AbortSignal.reason that might contain more specific context. Other parts of this PR preserve the reason (e.g., in token-refresh.ts). For consistency and better diagnostics, I suggest propagating the original reason. I've also reordered the checks to prioritize the more specific timeout error.

    if (clientCreationTimeoutController.signal.aborted) {
      throw new Error('Client creation timed out — check your authentication credentials');
    }
    if (abortSignal?.aborted) {
      throw abortSignal.reason ?? new Error('Aborted');
    }
    throw error;

Comment on lines +524 to +526
if (abortSignal?.aborted) {
throw new Error('Aborted');
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The catch block in runRoadmapGeneration discards the original abortSignal.reason on user cancellation, throwing a generic 'Aborted' error instead of preserving the specific reason.
Severity: MEDIUM

Suggested Fix

Modify the catch block to preserve the original abort reason. Instead of throw new Error('Aborted'), use a pattern like throw (abortSignal.reason instanceof Error ? abortSignal.reason : new Error('Aborted')) to ensure the specific reason is propagated.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: apps/desktop/src/main/ai/runners/roadmap.ts#L524-L526

Potential issue: In `runRoadmapGeneration`, when an operation is aborted via the
user-provided `abortSignal`, the `catch` block at line 524 discards the original abort
reason. It checks `if (abortSignal?.aborted)` and throws a generic `new
Error('Aborted')` instead of preserving the potentially more informative
`abortSignal.reason`. This is inconsistent with the abort-handling pattern used in other
parts of the codebase, such as `token-refresh.ts`. This results in the loss of
diagnostic context, as the user is shown a generic error message, making debugging more
difficult.

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/desktop/src/main/ai/auth/types.ts`:
- Around line 62-63: Add an optional AbortSignal parameter to
resolveAuthFromProviderQueue by changing its signature to accept abortSignal?:
AbortSignal, then forward that abortSignal into both resolveAuthFromQueue(...)
and the fallback resolveAuth(...) calls inside resolveAuthFromProviderQueue so
cancellation propagates to the queue path and the fallback path; mirror the
pattern used in factory.ts where resolveAuth calls receive the abortSignal.
Ensure parameter name matches abortSignal and thread it through any intermediate
calls within resolveAuthFromProviderQueue.

In `@apps/desktop/src/main/ai/runners/roadmap.ts`:
- Around line 41-50: The phase loops still catch aborts and treat them as normal
phase failures; update runDiscoveryPhase() and runFeaturesPhase() to use the
same abort-guard pattern as throwIfAborted/createSimpleClient: when catching
errors inside their loops, immediately rethrow if the provided AbortSignal is
aborted (or the caught error is an AbortError) instead of pushing it into errors
— i.e., call throwIfAborted(signal) at the start of each catch block (or check
signal?.aborted/instanceof Error reason and rethrow) so cancellation stops
execution immediately rather than being recorded as a retriable phase error.
- Around line 109-123: The function readAndRepairJsonFile currently swallows all
errors (I/O and parse) and returns null, which masks filesystem failures; change
it so only JSON parse/repair errors map to null while read/write/rename failures
are allowed to throw. Concretely: perform
readFileSync/repair/writeFileSync/renameSync and JSON.parse outside a broad
catch or rework the try/catch so that only JSON.parse (or a caught SyntaxError
from parsing the repaired string) is converted to a null return; do not catch or
suppress errors from readFileSync, writeFileSync, or renameSync. Reference:
readAndRepairJsonFile and callers such as loadPreservedFeatures to ensure they
still treat null as "invalid JSON" only.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c9c39732-e69f-4585-b52b-5d241087abf8

📥 Commits

Reviewing files that changed from the base of the PR and between 3f8e16e and b6717a3.

📒 Files selected for processing (13)
  • apps/desktop/src/main/__tests__/agent-queue.test.ts
  • apps/desktop/src/main/agent/agent-queue.ts
  • apps/desktop/src/main/ai/auth/resolver.ts
  • apps/desktop/src/main/ai/auth/types.ts
  • apps/desktop/src/main/ai/client/factory.ts
  • apps/desktop/src/main/ai/client/types.ts
  • apps/desktop/src/main/ai/providers/oauth-fetch.ts
  • apps/desktop/src/main/ai/runners/roadmap.test.ts
  • apps/desktop/src/main/ai/runners/roadmap.ts
  • apps/desktop/src/main/claude-profile/token-refresh.test.ts
  • apps/desktop/src/main/claude-profile/token-refresh.ts
  • apps/desktop/src/main/utils/__tests__/json-repair.test.ts
  • apps/desktop/src/main/utils/json-repair.ts

Comment on lines +62 to +63
/** Abort signal for cancellation during credential resolution */
abortSignal?: AbortSignal;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify resolveAuth call sites and whether abortSignal is forwarded.
# Expected: call paths that can be canceled should thread abortSignal into resolveAuth context.

rg -nP --type=ts '\bresolveAuth\s*\(' apps/desktop/src/main -C 3
rg -nP --type=ts '\bresolveAuthFromQueue\s*\(' apps/desktop/src/main -C 3
rg -nP --type=ts 'abortSignal\s*:' apps/desktop/src/main/ai/auth -C 2

Repository: AndyMik90/Aperant

Length of output: 24470


🏁 Script executed:

# Check the AuthResolverContext type definition to confirm abortSignal is optional
head -100 apps/desktop/src/main/ai/auth/types.ts | tail -50

Repository: AndyMik90/Aperant

Length of output: 1792


🏁 Script executed:

# Get more context around agent-manager.ts line 193
sed -n '180,210p' apps/desktop/src/main/agent/agent-manager.ts

Repository: AndyMik90/Aperant

Length of output: 1375


🏁 Script executed:

# Check resolveAuthFromQueue signature and how it handles abortSignal
sed -n '364,420p' apps/desktop/src/main/ai/auth/resolver.ts

Repository: AndyMik90/Aperant

Length of output: 2430


🏁 Script executed:

# List all callers of resolveAuth to identify which ones pass abortSignal
rg -nP 'resolveAuth\s*\(' apps/desktop/src/main -A 2 | grep -E '(resolveAuth|abortSignal)' | head -40

Repository: AndyMik90/Aperant

Length of output: 3200


🏁 Script executed:

# Check hasCredentials signature to see if it threads abortSignal
sed -n '320,335p' apps/desktop/src/main/ai/auth/resolver.ts

Repository: AndyMik90/Aperant

Length of output: 579


🏁 Script executed:

# Find where getResolvedAuth is called in agent-manager
rg -n 'getResolvedAuth\|resolveAuth' apps/desktop/src/main/agent/agent-manager.ts -B 2 -A 2

Repository: AndyMik90/Aperant

Length of output: 43


🏁 Script executed:

# Find where resolveAuthFromQueue calls resolveAuth internally
sed -n '364,500p' apps/desktop/src/main/ai/auth/resolver.ts | grep -A 20 -B 5 'resolveAuth'

Repository: AndyMik90/Aperant

Length of output: 829


🏁 Script executed:

# Check factory.ts full context around the resolveAuth calls at lines 135 and 276
sed -n '125,145p' apps/desktop/src/main/ai/client/factory.ts
sed -n '265,285p' apps/desktop/src/main/ai/client/factory.ts

Repository: AndyMik90/Aperant

Length of output: 1439


🏁 Script executed:

# Search for where getResolvedAuth is called in agent-manager
rg -n 'getResolvedAuth' apps/desktop/src/main/agent/agent-manager.ts -B 5 -A 5

Repository: AndyMik90/Aperant

Length of output: 43


🏁 Script executed:

# Check the full resolveAuthFromQueue function to find where resolveAuth is called
sed -n '364,650p' apps/desktop/src/main/ai/auth/resolver.ts | head -100

Repository: AndyMik90/Aperant

Length of output: 4084


🏁 Script executed:

# Check agent-manager imports and class definition to find getResolvedAuth signature
head -50 apps/desktop/src/main/agent/agent-manager.ts

Repository: AndyMik90/Aperant

Length of output: 2404


🏁 Script executed:

# Find the method containing line 193 in agent-manager
sed -n '150,200p' apps/desktop/src/main/agent/agent-manager.ts

Repository: AndyMik90/Aperant

Length of output: 2200


🏁 Script executed:

# Search for calls to that method to see if cancellation is needed
rg -n 'getResolvedAuth\|resolveAuthFromQueue' apps/desktop/src/main/agent/agent-manager.ts -B 10 -A 3

Repository: AndyMik90/Aperant

Length of output: 43


🏁 Script executed:

# Let's check line 150-200 more carefully to understand the context
wc -l apps/desktop/src/main/agent/agent-manager.ts
sed -n '140,200p' apps/desktop/src/main/agent/agent-manager.ts

Repository: AndyMik90/Aperant

Length of output: 2667


🏁 Script executed:

# Find the method signature that contains line 193
sed -n '130,145p' apps/desktop/src/main/agent/agent-manager.ts

Repository: AndyMik90/Aperant

Length of output: 598


🏁 Script executed:

# Search for all callers of the getResolvedAuth method
rg -n 'getResolvedAuth' apps/desktop/src/main -A 2 -B 2

Repository: AndyMik90/Aperant

Length of output: 43


🏁 Script executed:

# Check resolveCredentialsForAccount to confirm it threads abortSignal to resolveAuth
rg -n 'resolveCredentialsForAccount' apps/desktop/src/main/ai/auth/resolver.ts -A 20

Repository: AndyMik90/Aperant

Length of output: 1537


🏁 Script executed:

# Find all callers of resolveAuthFromProviderQueue to see if they have abortSignal context
rg -n 'resolveAuthFromProviderQueue' apps/desktop/src/main/agent/agent-manager.ts -B 5 -A 5

Repository: AndyMik90/Aperant

Length of output: 2412


🏁 Script executed:

# Search for abortSignal usage in agent-manager to understand cancellation patterns
rg -n 'abortSignal' apps/desktop/src/main/agent/agent-manager.ts -B 2 -A 2

Repository: AndyMik90/Aperant

Length of output: 43


🏁 Script executed:

# Check the full signature and callers of resolveAuthFromProviderQueue
sed -n '130,210p' apps/desktop/src/main/agent/agent-manager.ts

Repository: AndyMik90/Aperant

Length of output: 3349


🏁 Script executed:

# Check the signatures of the methods that call resolveAuthFromProviderQueue
sed -n '320,380p' apps/desktop/src/main/agent/agent-manager.ts
sed -n '420,480p' apps/desktop/src/main/agent/agent-manager.ts
sed -n '560,600p' apps/desktop/src/main/agent/agent-manager.ts

Repository: AndyMik90/Aperant

Length of output: 8017


🏁 Script executed:

# Search for abortSignal in the broader context of agent-manager tasks
rg -n 'abortSignal|abort|cancel' apps/desktop/src/main/agent/agent-manager.ts | head -20

Repository: AndyMik90/Aperant

Length of output: 43


🏁 Script executed:

# Check if TaskExecutionOptions includes abortSignal
rg -n 'TaskExecutionOptions\|AbortSignal' apps/desktop/src/main/agent/types.ts -A 10 | head -40

Repository: AndyMik90/Aperant

Length of output: 43


🏁 Script executed:

# Verify the full method signatures for the three calling methods
sed -n '310,330p' apps/desktop/src/main/agent/agent-manager.ts
sed -n '441,462p' apps/desktop/src/main/agent/agent-manager.ts
sed -n '550,570p' apps/desktop/src/main/agent/agent-manager.ts

Repository: AndyMik90/Aperant

Length of output: 2791


🏁 Script executed:

# Check if there's an abort/cancel controller at a higher level in agent-manager
rg -n 'AbortController|cancelTask|abortTask|stop' apps/desktop/src/main/agent/agent-manager.ts

Repository: AndyMik90/Aperant

Length of output: 549


🏁 Script executed:

# Verify factory.ts resolveAuth calls include abortSignal
sed -n '130,145p' apps/desktop/src/main/ai/client/factory.ts
sed -n '270,285p' apps/desktop/src/main/ai/client/factory.ts

Repository: AndyMik90/Aperant

Length of output: 1134


Thread abortSignal through resolveAuthFromProviderQueue() to enable cancellation on both queue and fallback auth paths.

resolveAuthFromProviderQueue() in agent-manager.ts (line 135) lacks an abortSignal parameter, so cancellation never reaches the fallback resolveAuth() call at line 193. Meanwhile, factory.ts properly threads abortSignal through resolveAuth() calls. Since startSpecCreation, startTaskExecution, and startQAProcess can be cancelled (via stopIdeation/stopRoadmap), auth resolution should respect cancellation.

Add abortSignal?: AbortSignal to resolveAuthFromProviderQueue() signature and pass it to both resolveAuthFromQueue() (line 176) and resolveAuth() (line 193) to match the pattern already established in factory.ts.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/desktop/src/main/ai/auth/types.ts` around lines 62 - 63, Add an optional
AbortSignal parameter to resolveAuthFromProviderQueue by changing its signature
to accept abortSignal?: AbortSignal, then forward that abortSignal into both
resolveAuthFromQueue(...) and the fallback resolveAuth(...) calls inside
resolveAuthFromProviderQueue so cancellation propagates to the queue path and
the fallback path; mirror the pattern used in factory.ts where resolveAuth calls
receive the abortSignal. Ensure parameter name matches abortSignal and thread it
through any intermediate calls within resolveAuthFromProviderQueue.

Comment on lines +41 to +50
function throwIfAborted(signal?: AbortSignal): void {
if (!signal?.aborted) return;

const reason = signal.reason;
if (reason instanceof Error) {
throw reason;
}

throw new Error(typeof reason === 'string' ? reason : 'Aborted');
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Cancellation still turns into phase retries after startup.

This new wrapper only aborts createSimpleClient(). Once the client exists, both phase loops still catch abort errors and append them to errors, so canceling during discovery/features comes back as a normal phase failure after retrying instead of stopping immediately.

🛠️ Suggested guard for both phase loops
  for (let attempt = 0; attempt < MAX_RETRIES; attempt++) {
+    throwIfAborted(abortSignal);
     ...
     } catch (error) {
+      throwIfAborted(abortSignal);
       errors.push(`Attempt ${attempt + 1}: ${error instanceof Error ? error.message : String(error)}`);
     }
  }

Apply the same pattern in both runDiscoveryPhase() and runFeaturesPhase().

Also applies to: 500-533

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/desktop/src/main/ai/runners/roadmap.ts` around lines 41 - 50, The phase
loops still catch aborts and treat them as normal phase failures; update
runDiscoveryPhase() and runFeaturesPhase() to use the same abort-guard pattern
as throwIfAborted/createSimpleClient: when catching errors inside their loops,
immediately rethrow if the provided AbortSignal is aborted (or the caught error
is an AbortError) instead of pushing it into errors — i.e., call
throwIfAborted(signal) at the start of each catch block (or check
signal?.aborted/instanceof Error reason and rethrow) so cancellation stops
execution immediately rather than being recorded as a retriable phase error.

Comment on lines +109 to +123
function readAndRepairJsonFile<T>(filePath: string): T | null {
try {
const raw = readFileSync(filePath, 'utf-8');
const repaired = repairJson(raw);

if (repaired !== raw) {
const tmpFile = `${filePath}.tmp.${process.pid}`;
writeFileSync(tmpFile, repaired, 'utf-8');
renameSync(tmpFile, filePath);
}

return JSON.parse(repaired) as T;
} catch {
return null;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don’t collapse repair write failures into “invalid JSON”.

readAndRepairJsonFile() currently returns null for syntax problems and for readFileSync/writeFileSync/renameSync failures. That same null is treated as “no preserved features” by loadPreservedFeatures(), so an I/O error while persisting a repaired roadmap can silently drop existing planned/in-progress/done items on the next merge. Only map parse failures to null; let filesystem failures surface.

🛠️ Proposed fix
 function readAndRepairJsonFile<T>(filePath: string): T | null {
-  try {
-    const raw = readFileSync(filePath, 'utf-8');
-    const repaired = repairJson(raw);
-
-    if (repaired !== raw) {
-      const tmpFile = `${filePath}.tmp.${process.pid}`;
-      writeFileSync(tmpFile, repaired, 'utf-8');
-      renameSync(tmpFile, filePath);
-    }
-
-    return JSON.parse(repaired) as T;
-  } catch {
-    return null;
+  const raw = readFileSync(filePath, 'utf-8');
+  const repaired = repairJson(raw);
+
+  try {
+    const parsed = JSON.parse(repaired) as T;
+
+    if (repaired !== raw) {
+      const tmpFile = `${filePath}.tmp.${process.pid}`;
+      writeFileSync(tmpFile, repaired, 'utf-8');
+      renameSync(tmpFile, filePath);
+    }
+
+    return parsed;
+  } catch (error) {
+    if (error instanceof SyntaxError) {
+      return null;
+    }
+    throw error;
   }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
function readAndRepairJsonFile<T>(filePath: string): T | null {
try {
const raw = readFileSync(filePath, 'utf-8');
const repaired = repairJson(raw);
if (repaired !== raw) {
const tmpFile = `${filePath}.tmp.${process.pid}`;
writeFileSync(tmpFile, repaired, 'utf-8');
renameSync(tmpFile, filePath);
}
return JSON.parse(repaired) as T;
} catch {
return null;
}
function readAndRepairJsonFile<T>(filePath: string): T | null {
const raw = readFileSync(filePath, 'utf-8');
const repaired = repairJson(raw);
try {
const parsed = JSON.parse(repaired) as T;
if (repaired !== raw) {
const tmpFile = `${filePath}.tmp.${process.pid}`;
writeFileSync(tmpFile, repaired, 'utf-8');
renameSync(tmpFile, filePath);
}
return parsed;
} catch (error) {
if (error instanceof SyntaxError) {
return null;
}
throw error;
}
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/desktop/src/main/ai/runners/roadmap.ts` around lines 109 - 123, The
function readAndRepairJsonFile currently swallows all errors (I/O and parse) and
returns null, which masks filesystem failures; change it so only JSON
parse/repair errors map to null while read/write/rename failures are allowed to
throw. Concretely: perform readFileSync/repair/writeFileSync/renameSync and
JSON.parse outside a broad catch or rework the try/catch so that only JSON.parse
(or a caught SyntaxError from parsing the repaired string) is converted to a
null return; do not catch or suppress errors from readFileSync, writeFileSync,
or renameSync. Reference: readAndRepairJsonFile and callers such as
loadPreservedFeatures to ensure they still treat null as "invalid JSON" only.

Comment on lines 216 to +222

try {
return await refreshOAuthToken(stored.refresh_token, providerSpec, tokenFilePath);
return await refreshOAuthToken(stored.refresh_token, providerSpec, tokenFilePath, abortSignal);
} catch (err) {
if (abortSignal?.aborted) {
throw (abortSignal.reason instanceof Error ? abortSignal.reason : new Error('Aborted'));
}

This comment was marked as outdated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant